feat: migrate x/staking to app wiring #12102

JeancarloBarrios · 2022-05-31T20:10:01Z

Description

Migrate x/staking to the new app wiring (dependency injection).

Tracking progress at #12036

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

x/staking/module.go

x/bank/module.go

api/cosmos/staking/module/v1/module.pulsar.go

+	protoiface "google.golang.org/protobuf/runtime/protoiface"
+	protoimpl "google.golang.org/protobuf/runtime/protoimpl"
+	io "io"
+	reflect "reflect"


blushi

is this R4R?

proto/cosmos/staking/module/v1/module.proto

x/staking/module.go

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

JeancarloBarrios · 2022-06-02T16:29:01Z

R4R

I still have this test regarding the distribution module for delegation I'm getting the error no delegation distribution info

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

- hack: Fixes post construction SetHooks mutation

kocubinski · 2022-06-02T22:19:36Z

x/staking/keeper/keeper.go

@@ -63,14 +63,12 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger {
 }

 // Set the validator hooks
-func (k *Keeper) SetHooks(sh types.StakingHooks) *Keeper {


This pattern makes little sense to me given the name SetHooks.

I think we should add an API-breaking changelog entry, iirc Osmosis is using SetHooks pretty heavily

kocubinski · 2022-06-02T22:28:01Z

x/staking/keeper/keeper.go

@@ -47,7 +47,7 @@ func NewKeeper(
 		panic(fmt.Sprintf("%s module account has not been set", types.NotBondedPoolName))
 	}

-	return Keeper{


Fundamentally this change is required so that when InitGenesis iterates over the ModuleManager the StakingKeeper which is retrieved (by reference) is the same area of memory which the post construction SetHooks mutation modifies.

This is the reason for now passing the Keeper around everywhere by reference.

Not a huge fan of this, but it seems it will be up for refactor when we get to x/distribution and x/slashing, or possibly sooner. Those modules can provide their StakingHooks for this module to order as BaseAppOptions.

JeancarloBarrios · 2022-06-02T22:35:02Z

@aaronc, @kocubinski found a memory bug regarding the staking keeper. Given that the new mechanism was initialized before the hooks are added to it, it will initialize with hooks on nil. @kocubinski solution is in the comment above. I was wondering if you could give it a look and give us your opinion about it.

aaronc

LGTM

…_use_app_wiring' into JeancarloBarrios/migrate_staking_use_app_wiring

codecov · 2022-06-02T23:49:37Z

Codecov Report

Merging #12102 (d8903e2) into main (fe5c22e) will increase coverage by 0.00%.
The diff coverage is 94.28%.

@@           Coverage Diff           @@
##             main   #12102   +/-   ##
=======================================
  Coverage   66.00%   66.00%           
=======================================
  Files         671      671           
  Lines       71006    71011    +5     
=======================================
+ Hits        46866    46872    +6     
+ Misses      21478    21477    -1     
  Partials     2662     2662

Impacted Files	Coverage Δ
simapp/export.go	`11.76% <0.00%> (ø)`
x/distribution/module.go	`62.85% <ø> (ø)`
x/staking/genesis.go	`46.15% <0.00%> (ø)`
x/staking/keeper/grpc_query.go	`58.95% <ø> (ø)`
x/staking/keeper/invariants.go	`62.40% <83.33%> (ø)`
x/staking/module.go	`70.58% <93.75%> (+5.79%)`	⬆️
simapp/app.go	`78.23% <100.00%> (-0.51%)`	⬇️
x/bank/module.go	`63.91% <100.00%> (ø)`
x/distribution/simulation/operations.go	`80.21% <100.00%> (ø)`
x/slashing/module.go	`61.42% <100.00%> (ø)`
... and 8 more

amaury1093

🎉

@JeancarloBarrios could you create a quick follow-up to add changelog entries?

amaury1093 · 2022-06-03T08:48:20Z

x/staking/keeper/keeper.go

@@ -63,14 +63,12 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger {
 }

 // Set the validator hooks
-func (k *Keeper) SetHooks(sh types.StakingHooks) *Keeper {


I think we should add an API-breaking changelog entry, iirc Osmosis is using SetHooks pretty heavily

amaury1093 · 2022-06-03T08:48:28Z

x/staking/keeper/keeper.go

@@ -47,7 +47,7 @@ func NewKeeper(
 		panic(fmt.Sprintf("%s module account has not been set", types.NotBondedPoolName))
 	}

-	return Keeper{


## Description Migrate x/staking to the new app wiring (dependency injection). Tracking progress at cosmos#12036 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [x] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

JeancarloBarrios added 3 commits May 31, 2022 14:04

feat: migrate x/staking to app wiring

f272fc8

add staking to app yaml

0530a1c

modified staking module

15446da

github-actions bot added the C:x/staking label May 31, 2022

JeancarloBarrios added 6 commits May 31, 2022 16:35

partial simapp changes

12d1ec4

generated pulsar

ebeb7c5

pre merge

fbbbd46

Merge branch 'main' into JeancarloBarrios/migrate_staking_use_app_wiring

b467dcd

simapp app

6046ac5

add depinject struct for parmeters

39eb9a9

blushi reviewed Jun 2, 2022

View reviewed changes

x/staking/module.go Outdated Show resolved Hide resolved

Add depinject key to bankKeeper

c0eaec6

github-actions bot added the C:x/bank label Jun 2, 2022

blushi reviewed Jun 2, 2022

View reviewed changes

x/bank/module.go Outdated Show resolved Hide resolved

blushi mentioned this pull request Jun 2, 2022

feat: migrate x/genutil to app wiring #12124

Merged

19 tasks

PR changes

834fd1a

github-advanced-security bot found potential problems Jun 2, 2022

View reviewed changes

blushi reviewed Jun 2, 2022

View reviewed changes

proto/cosmos/staking/module/v1/module.proto Outdated Show resolved Hide resolved

x/staking/module.go Outdated Show resolved Hide resolved

Update proto/cosmos/staking/module/v1/module.proto

ed83c41

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

JeancarloBarrios and others added 8 commits June 2, 2022 10:29

Update x/staking/module.go

7bb92f1

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

PR changes

722a05e

update pulsar

16a6a40

PR changes

6e8a954

Merge branch 'main' into JeancarloBarrios/migrate_staking_use_app_wiring

7b8c22f

Merge branch 'main' into JeancarloBarrios/migrate_staking_use_app_wiring

194538c

merge changes

2005d75

Pass StakingKeeper by reference everywhere

1dc2759

- hack: Fixes post construction SetHooks mutation

kocubinski reviewed Jun 2, 2022

View reviewed changes

aaronc approved these changes Jun 2, 2022

View reviewed changes

JeancarloBarrios added 3 commits June 2, 2022 16:38

change some keeper value to reference for the hooks fix

215b63f

change test tools to recieve pointer insted of instance

cd880e4

change test tools to recieve pointer insted of instance

9ec5a3c

github-actions bot added C:Simulations C:x/distribution distribution module related labels Jun 2, 2022

kocubinski and others added 2 commits June 2, 2022 18:23

Fix tests, more pass by reference

b241d51

change test tools to recieve pointer insted of instance

35440b7

github-actions bot added the C:x/slashing label Jun 2, 2022

Merge remote-tracking branch 'origin/JeancarloBarrios/migrate_staking…

d8903e2

…_use_app_wiring' into JeancarloBarrios/migrate_staking_use_app_wiring

JeancarloBarrios marked this pull request as ready for review June 2, 2022 23:45

JeancarloBarrios requested a review from a team as a code owner June 2, 2022 23:45

JeancarloBarrios mentioned this pull request Jun 3, 2022

migrate x/staking to use app wiring #12036

Closed

4 tasks

blushi approved these changes Jun 3, 2022

View reviewed changes

blushi added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 3, 2022

mergify bot merged commit 90d6745 into main Jun 3, 2022

mergify bot deleted the JeancarloBarrios/migrate_staking_use_app_wiring branch June 3, 2022 08:10

amaury1093 reviewed Jun 3, 2022

View reviewed changes

JeancarloBarrios mentioned this pull request Jun 3, 2022

fix: added breaking change in staking keeper api #12139

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate x/staking to app wiring #12102

feat: migrate x/staking to app wiring #12102

JeancarloBarrios commented May 31, 2022 •

edited by blushi

Loading

blushi left a comment

JeancarloBarrios commented Jun 2, 2022

kocubinski Jun 2, 2022 •

edited

Loading

amaury1093 Jun 3, 2022

kocubinski Jun 2, 2022 •

edited

Loading

amaury1093 Jun 3, 2022

JeancarloBarrios commented Jun 2, 2022

aaronc left a comment

codecov bot commented Jun 2, 2022

amaury1093 left a comment •

edited

Loading

amaury1093 Jun 3, 2022

amaury1093 Jun 3, 2022

feat: migrate x/staking to app wiring #12102

feat: migrate x/staking to app wiring #12102

Conversation

JeancarloBarrios commented May 31, 2022 • edited by blushi Loading

Description

Author Checklist

Reviewers Checklist

blushi left a comment

Choose a reason for hiding this comment

JeancarloBarrios commented Jun 2, 2022

kocubinski Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

amaury1093 Jun 3, 2022

Choose a reason for hiding this comment

kocubinski Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

amaury1093 Jun 3, 2022

Choose a reason for hiding this comment

JeancarloBarrios commented Jun 2, 2022

aaronc left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 2, 2022

Codecov Report

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

amaury1093 Jun 3, 2022

Choose a reason for hiding this comment

amaury1093 Jun 3, 2022

Choose a reason for hiding this comment

JeancarloBarrios commented May 31, 2022 •

edited by blushi

Loading

kocubinski Jun 2, 2022 •

edited

Loading

kocubinski Jun 2, 2022 •

edited

Loading

amaury1093 left a comment •

edited

Loading